@pmachapman dismissed @github-advanced-security[bot] from 3 discussions.
Reviewable status: 0 of 19 files reviewed, all discussions resolved

Partially reviewed; have a few comments so far.

@Nateowami reviewed 1 of 19 files at r1, 17 of 18 files at r2, 13 of 18 files at r3.
Reviewable status: 14 of 19 files reviewed, 6 unresolved discussions (waiting on @pmachapman)

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 42 at r3 (raw file):

        string scriptureRange,
        string targetProjectId,
        DateTime timestamp

Why is the timestamp required? Shouldn't "latest" be the default?

src/SIL.XForge.Scripture/Services/DeltaUsxMapper.cs line 180 at r3 (raw file):

    public static string ExtractBookId(string usfm)
    {
        string firstLine = usfm.Split('\n').FirstOrDefault()?.Trim() ?? string.Empty;

Isn't splitting the string allocating a fairly large string array? In my testing there is a project where the Psalms alone is 5MB. Of course that's an outlier, but there are 30 projects where the Psalms USFM file is at least one million bytes.

Maybe something like:

        int end = usfm.IndexOfAny(['\r', '\n']);
        string firstLine = end >= 0 ? usfm[..end] : usfm;
        var match = BookIdRegex().Match(firstLine);
        return match.Success ? match.Groups[1].Value : string.Empty;

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 33 at r3 (raw file):

using TextInfo = SIL.XForge.Scripture.Models.TextInfo;

#pragma warning disable CA2254

I don't think this is actually needed? And if it is it would be good to have a comment saying what warning it pertains to and why it needs to be turned off.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 153 at r3 (raw file):

                await hubContext.NotifyDraftApplyProgress(
                    sfProjectId,
                    new DraftApplyState { State = $"Retrieving draft for {Canon.BookIdToEnglishName(book)}." }

Shouldn't we be setting machine-readable values that can be parsed and then displayed however the client chooses (e.g. progress bar, or localized messages)?

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 170 at r3 (raw file):

                }

                // Ensure that if chapters is blank, it contains every chapter in the book

What does this mean? Is this handling a versification mismatch?

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 179 at r3 (raw file):

                // Store the USJ for each chapter, so if we download form Serval we only do it once per book
                List<Usj> chapterUsj = [];
                foreach (int chapterNum in chapters.Where(c => c > 0))

Is a chapter zero conceptually invalid?

Reviewable status: 14 of 19 files reviewed, 5 unresolved discussions (waiting on @Nateowami)

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 42 at r3 (raw file):

Timestamp should be whatever the build generated timestamp is. As users can apply older builds, and this API is for internal use only, I figured it was best to keep the timestamp required to reduce guesswork around what is happening.

src/SIL.XForge.Scripture/Services/DeltaUsxMapper.cs line 180 at r3 (raw file):

Done. Yes, you are right. I've reviewed Paratext's code and it doesn't grab the first line, it just regex's for the id in the entire USFM, so I will do that (the source generated Regex will do that efficiently.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 33 at r3 (raw file):

It is only present to remove an unwanted code analysis notice. I'll add a comment, like it has in MachineProjectService.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 153 at r3 (raw file):

Probably the best way to handle the localization is what we did for sync progress where there is a phase enum that changes based on the what the phase the draft application is in.

I wasn't going to do that in this PR, as I am not sure how much this logic will change after testing and some real world use, and until the frontend is done, I'm really not sure how the status will be displayed to the user (Maybe percentage complete? Maybe queued/running/success/error?). Either way, the value that feed this will be emitted via a number or enum in the DraftApplyState.

I think at this state these messages won't be displayed on the frontend unless something goes wrong. In that case, they will be sent in an error report or similar.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 170 at r3 (raw file):

I've added a clarifying comment.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 179 at r3 (raw file):

In Paratext, chapter 0 means all chapters. There are no chapter zeros in the projects we have on live.

@Nateowami reviewed 2 of 18 files at r3, 13 of 18 files at r5, 2 of 2 files at r6, 2 of 2 files at r7.
Reviewable status: 18 of 19 files reviewed, 8 unresolved discussions (waiting on @pmachapman)

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 154 at r7 (raw file):

                await hubContext.NotifyDraftApplyProgress(
                    sfProjectId,
                    new DraftApplyState { State = $"Retrieving draft for {Canon.BookIdToEnglishName(book)}." }

Personally I would prefer Canon.BookNumberToId for logging warnings.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 222 at r7 (raw file):

                                    }
                                );
                                break;

Shouldn't this also call result.Failures.Add(book);?

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 330 at r7 (raw file):

        try
        {
            // Being the transaction

I think it's supposed to say "Begin"

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 416 at r7 (raw file):

                {
                    string bookId = Canon.BookNumberToId(bookNum);
                    if (result.Failures.LastOrDefault() != bookId)

I thought result.Failures lists chapters that failed to apply (that's what the doc comment on it says), so I wouldn't expect to find the book ID in the list. Or maybe the doc comment is wrong?

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 447 at r7 (raw file):

                    {
                        // Only list the book failure once
                        result.Failures.Add(bookId);

Maybe the failure list should be a set instead of an array?

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 58 at r7 (raw file):

                )
            );
            return Ok();

Maybe we should return the job identifier when an API method starts a job? Or maybe there's no good reason to do that, since we don't have a good way of checking when a job is finished?

It seems like one strategy could be to have SignalR broadcast completion of jobs by ID, and then the front end could invoke a method that starts a job, and then just wait to hear that the job with that ID is complete.

Sorry for slowly trickling review comments.

@Nateowami reviewed 1 of 18 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pmachapman)

Reviewable status: 0 of 19 files reviewed, 6 unresolved discussions (waiting on @Nateowami)

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 58 at r7 (raw file):

Maybe we should return the job identifier when an API method starts a job?

I've added this as I think it might be useful in future.

It seems like one strategy could be to have SignalR broadcast completion of jobs by ID, and then the front end could invoke a method that starts a job, and then just wait to hear that the job with that ID is complete.

It can be watched by projectId, and then SignalR watches for the success or failure flag to be set. The jobId can't be passed into the running process, so I can't really see how to have SignalR emit that.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 154 at r7 (raw file):

Done - I now just return the bookId.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 222 at r7 (raw file):

Done. Good spotting - thanks.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 330 at r7 (raw file):

Done. Yes! Thanks!

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 416 at r7 (raw file):

I've updated the book comment. If the failure occurs before we get to the chapter (i.e. the user cannot write a book at all, then it will just have the book id.

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 447 at r7 (raw file):

Thanks for the idea - that has simplified it.

@Nateowami reviewed 1 of 19 files at r8, 12 of 16 files at r9, 1 of 2 files at r11, 5 of 5 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 33 at r3 (raw file):

I don't think it's present on my machine. Not sure what tool is creating it.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 33 at r3 (raw file):

Visual Studio 2022's Code Analysis